Specialize GraphOps #809
Specialize GraphOps #809
Conversation
* only include graph_ops.hpp from split_dbm.cpp * pass function to Heap, remove type parameter * use inline thread_local static members * add WeightIndexable concept, preparing switch from template to a function (using operator() instead of operator[]) * inline classes dedicated to operator overloading * add consts Signed-off-by: Elazar Gershuni <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (1)
src/main/linux_verifier.cpp (1)
Line range hint
12-12: Eliminate redundant 'std::memset' on zero-initialized 'attr'The
attrvariable is already zero-initialized withunion bpf_attr attr {};. The subsequentstd::memset(&attr, '\0', sizeof(attr));is redundant and can be removed to clean up the code.Apply this diff to remove the redundant
std::memset:union bpf_attr attr {}; -std::memset(&attr, '\0', sizeof(attr));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
src/crab/split_dbm.cpp(10 hunks)src/crab/split_dbm.hpp(2 hunks)src/crab_utils/graph_ops.hpp(26 hunks)src/crab_utils/heap.hpp(4 hunks)src/main/linux_verifier.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/crab/split_dbm.hpp (1)
Learnt from: elazarg
PR: vbpf/ebpf-verifier#689
File: src/crab/split_dbm.cpp:910-910
Timestamp: 2024-11-12T09:50:16.890Z
Learning: Avoid suggesting changes that are unrelated to the current PR.
src/crab/split_dbm.cpp (1)
Learnt from: elazarg
PR: vbpf/ebpf-verifier#689
File: src/crab/split_dbm.cpp:910-910
Timestamp: 2024-11-12T09:50:16.890Z
Learning: Avoid suggesting changes that are unrelated to the current PR.
🔇 Additional comments (15)
src/crab_utils/heap.hpp (1)
51-52: Verify 'percolateDown' usage after parameter removal
The percolateDown method no longer accepts an index parameter and defaults i to 0. Confirm that there are no other calls requiring a different starting index, as this change limits the method's flexibility.
Run the following script to search for calls to percolateDown with arguments:
✅ Verification successful
Parameter removal from percolateDown is safe
The only usage of percolateDown() in the codebase is within the same class, called without arguments. The change aligns with its actual usage pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for calls to 'percolateDown' that pass an argument.
# Expectation: No calls to 'percolateDown' should pass an argument.
rg --no-heading 'percolateDown\s*\([^)]+'
Length of output: 43
Script:
#!/bin/bash
# Let's search for all occurrences of percolateDown to understand its usage
rg -A 2 'percolateDown'
# Also search for the class definition to understand the context
ast-grep --pattern 'class $_ {
$$$
percolateDown() {
$$$
}
$$$
}'
Length of output: 384
src/crab/split_dbm.hpp (2)
129-129: LGTM: Good separation of declaration and implementation.
Moving the implementation to the cpp file improves compilation times and encapsulation.
304-304: LGTM: Good separation of declaration and implementation.
Moving the implementation to the cpp file improves compilation times and encapsulation.
src/crab/split_dbm.cpp (12)
9-9: LGTM: Required include for GraphOps namespace.
282-284: LGTM: Good use of specialized GraphOps namespace.
Explicit namespace usage improves code clarity and maintainability.
505-507: LGTM: Consistent use of GraphOps namespace.
526-528: LGTM: Consistent use of GraphOps namespace.
533-533: LGTM: Consistent use of GraphOps namespace.
721-722: LGTM: Good initialization practice.
Proper initialization of is_closed variable and consistent namespace usage.
727-727: LGTM: Consistent use of GraphOps namespace.
733-737: LGTM: Consistent use of GraphOps namespace.
Line range hint 882-892: LGTM: Good use of edge_vector for batch operations.
Efficient batch processing of edge updates using GraphOps::edge_vector.
894-894: LGTM: Consistent use of GraphOps namespace.
930-935: LGTM: Clean implementation of specialized methods.
Good delegation to GraphOps namespace, maintaining separation of concerns.
946-951: LGTM: Good use of edge_vector and consistent namespace usage.
Efficient batch processing of edge updates and proper use of GraphOps namespace.
| Comparator lt; | ||
| std::vector<int> heap; // heap of ints | ||
| std::vector<int> indices; // int -> index in heap | ||
| std::function<bool(int, int)> lt; // |
There was a problem hiding this comment.
Performance impact due to 'std::function' in comparator
Replacing the template comparator with std::function<bool(int, int)> lt introduces runtime overhead from type erasure and potential heap allocations. If performance is critical, consider reverting to a template or using inline function objects to maintain optimal efficiency.
graph_ops.hpp is a large header with too many type parameters and implementation details, and is only needed for SplitDBM.